-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cli parser fixups #116
Cli parser fixups #116
Conversation
This reverts commit c289504.
@@ -26,6 +26,9 @@ | |||
"preLaunchTask": "", //"build", | |||
"program": "${workspaceFolder}/out/dotnet/source/Mlos.Agent.Server/objd/AnyCPU/Mlos.Agent.Server.dll", | |||
"args": [ | |||
//"--optimizer-uri", | |||
//"http://localhost:50051", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixup for the default debugger experience.
If this options are enabled by default, the command will fail.
In the future we can consider to add a preLaunchTask
that starts the optimizer service.
|
||
var cliOptsParseResult = CommandLine.Parser.Default.ParseArguments<CliOptions>(args) | ||
var cliOptsParser = new Parser(with => with.HelpWriter = null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We add custom help output handling with ShowUsageHelp
now.
/// See Also: https://github.com/microsoft/MLOS/issues/112. | ||
/// </remarks> | ||
[CommandLine.Value(0)] | ||
public IEnumerable<string> ExtraArgs { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option unfortunately isn't hidden and leaves a weird value pos. 0
text at the end of the help output, but it does allow us to detect the extra arguments.
…s well like having ExtraArgs
This looks reasonable but you shouldn't trust my judgement of C#. My impulse (which you should feel free to ignore) would be to ask for a test to show that it errors on extra args. |
Not a bad idea. Will have to think about how to implement that though. |
- move cli parsing to a separate file - change error message for unknown extra arguments
note that parsing of the right arguments already happens in other unit tests This invokation method appears to work on both windows and linux
Added this in 2ec178a |
…rguments" This reverts commit 2ec178a.
Fixes #112